Skip to content

Padded encoded ULIDs to canonical length#11

Merged
rkadaj merged 1 commit into
masterfrom
rkadaj/ensure-encoded-ulid-is-26-chars
Jun 4, 2026
Merged

Padded encoded ULIDs to canonical length#11
rkadaj merged 1 commit into
masterfrom
rkadaj/ensure-encoded-ulid-is-26-chars

Conversation

@rkadaj
Copy link
Copy Markdown

@rkadaj rkadaj commented Jun 4, 2026

Right justify encoded ULID to ensure 26 character length regardless of initial value

@rkadaj rkadaj marked this pull request as ready for review June 4, 2026 18:59
@rkadaj rkadaj changed the title Paded encoded ULIDs to canonical length Padded encoded ULIDs to canonical length Jun 4, 2026
@rkadaj rkadaj force-pushed the rkadaj/ensure-encoded-ulid-is-26-chars branch from aadda48 to 7b2b615 Compare June 4, 2026 19:08
Copy link
Copy Markdown
Member

@martelogan martelogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be good 👍 ; mentioned in a comment (and per slack) that I believe the prior version was just inherited from some encode32 micro-optimizations which asumed the shortest possible underlying ULID representation before serializing

Comment thread lib/ulid/generate.rb
return "0#{b32}" if b32.length == 25

b32
b32.rjust(26, "0")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 , per discussion on slack, this should be fine as I believe the prior version was just a micro-optimization assuming a single leading zero for 25-char ULIDs generated using the shortest possible representation, but there should be no issue to replace with rjust(26, "0") to handle an arbitrary number of leading zeroes for alternative underlying representations

Copy link
Copy Markdown

@drobin03 drobin03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for upstreaming this! 🚀

@rkadaj rkadaj merged commit 82b9a67 into master Jun 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants